-
Notifications
You must be signed in to change notification settings - Fork 35
chore(price-feed): added update parameters as a column in each table for EVM #739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Update Parameters for BOLD: **1 hour heartbeat or 0.5% price deviation** | ||
|
||
Update Parameters for the other assets: **1 hour heartbeat or 2% price deviation** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant now and can be removed. Same applies to other places in the file.
Signed-off-by: nidhi-singh02 <nidhi2894@gmail.com>
f5d8d43
to
0a723fc
Compare
Signed-off-by: nidhi-singh02 <nidhi2894@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
important stuff LGTM, mostly nits / minor suggestions that I think may make the code a bit easier to maintain
const paramCounts = feeds.reduce((acc, feed) => { | ||
acc[feed.updateParameters] = (acc[feed.updateParameters] || 0) + 1; | ||
return acc; | ||
}, {} as Record<string, number>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super minor nit but I tend to find this kind of thing is more easy to understand when implemented as:
const paramCounts = Object.fromEntries(
Object
.entries(Object.groupBy(feeds, feed => feed.updateParameters))
.map(([updateParameters, feeds]) => [updateParameters, feeds.length])
)
I will also typically create a mapValues
helper (which really should be in the ES standard IMO and probably will be eventually) which I use all the time and makes the code more concise:
const paramCounts = mapValues(
Object.groupBy(feeds, feed => feed.updateParameters)),
feeds => feeds.length
)
For some reason, reduce
seems to be a hard operation for a lot of folks to easily grok so I've started to move away from using it except in very specific scenarios. Here is a decent writeup for an eslint rule that I use nowadays that has some links to some threads on the topic.
// Calculate table height based on number of items | ||
// Each row is approximately 40px (py-2 = 8px top + 8px bottom + content height) | ||
// Header is approximately 48px (py-2 = 8px top + 8px bottom + font height) | ||
// Show 7 rows by default, then scroll - but maintain consistent minimum height | ||
const maxVisibleRows = 7; | ||
const shouldScroll = feeds.length > maxVisibleRows; | ||
const rowHeight = 56; // Increased row height to account for actual content height | ||
const headerHeight = 48; // Header height in pixels | ||
const exactTableHeight = `${headerHeight + maxVisibleRows * rowHeight}px`; // Exact height for 7 rows | ||
const tableHeight = shouldScroll ? exactTableHeight : "auto"; // Use exact height for scrollable tables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dislike this kind of thing, it's very brittle (i.e. it's a pain to ensure every change is tested across device sizes and it's easy to do something that changes the calculations and not know it breaks the UI) and it's very easy to run into render issues on different platforms where size calculations have slight differences.
It also feels unnecessarily complex for what you're looking to accomplish.
In this case, what I'd suggest doing is just adding the max-height
css property to the table
<span className="font-medium">Exception:</span> | ||
<span | ||
dangerouslySetInnerHTML={{ | ||
__html: params.replace("<br/>", " / "), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit strange to me, is there any reason why params
here needs to be free text with html embedded? This is not coming from a CMS or any external source, so there shouldn't be any reason to pass around html as strings. I'd suggest changing the format of updateParameters
-- either pass around jsx fragments, or better yet, since it looks like the only difference in the entries is the heartbeat length & the deviation, just make updateParameters
something like { heartbeatLength: number, deviation: number }
) : ( | ||
<div className="flex items-start gap-1.5 text-gray-400 text-xs"> | ||
<div className="w-1.5 h-1.5 bg-green-500 rounded-full mt-1 flex-shrink-0"></div> | ||
<span>Same as above</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'm not a fan of this. I know the repetition is obnoxious, but when I'm looking up data, I'd much rather see repetition, but to have the value I'm looking for right where I'm looking, than to see "same as above" and then have to go cross reference another row.
My suggestion would be to just use the isFirstInGroup
style for every entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, then shouldn't we keep these update parameters in the top line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if we do that, then the experience wouldn't be consistent between all the table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow but from the screenshot I don't see any reason to get rid of the top line or the green/red dots, it's a helpful way to highlight the data that differs from baseline.
Description
Earlier the update parameters were mentioned at the top for each of the chain, which was not easy to refer if you are at a particular asset in the list and the table is large.
Added a new column for "Update parameters" makes it easy for developers to access it readily.
Type of Change
Areas Affected
This page is being modified https://docs.pyth.network/price-feeds/sponsored-feeds/evm
Checklist
pre-commit run --all-files
to check for linting errorsRelated Issues
Closes #
Additional Notes
Contributor Information
Screenshots